Detect an unconditional UPDATE with sqlparse instead of splitting on whitespace - #1614
Detect an unconditional UPDATE with sqlparse instead of splitting on whitespace#1614VXNCXNX wants to merge 2 commits into
Conversation
|
Nice catch, and thank you for fixing it! I can confirm the bug and the fix from my fork. Against current main, with The bare I cherry-picked your commit and re-ran the same cases:
The subquery case is a nice bonus of using the parse tree: a WHERE that only Full suite green on my side too. Hope this helps it along. |
81b1c9b to
6161e22
Compare
|
Rebased onto current On the new base: |
Description
With
destructive_warningcoveringunconditional_update, an UPDATE with no WHERE clause skips the confirmation prompt if any string literal in it contains the standalone word "where".Repro
Run on this checkout, not reasoned about:
The second one is an unconditional UPDATE. It returns False, so the prompt never appears.
The fix
query_is_unconditional_updatedidformatted_sql.split()and checked"where" not in tokens. Splitting on whitespace turns the contents of a string literal into tokens, so'no where clause here'yields a barewhere.It now uses sqlparse's parse tree, which the module already imports: confirm the first meaningful token is the DML keyword UPDATE, then look for a
sqlparse.sql.Whereamong the statement's top-level tokens. Since sqlparse handles case and comments itself, the call site passes the raw query rather than the lowercased comment-stripped one.query_starts_withis untouched, it serves the separate keyword-prefix path and is not affected.A WHERE inside a subquery is nested under the Parenthesis, not a top-level token, so
update t set c = (select x from y where z = 1)correctly still warns.Verification
Checked against these, all as expected:
Added a parametrized case list to
tests/parseutils/test_parseutils.pynext to the existingtest_is_destructive. It fails against the old implementation on the string-literal and subquery cases.pytest tests/parseutils/passes, 115 tests.One thing I did not change:
with x as (...) update t set c = 1returns False, because the first token is the CTE rather than the DML keyword. That is the same as before this patch, so it is a separate gap rather than a regression, and I left it alone.Checklist
changelog.rst.AUTHORSfile (or it's already there).pip install pre-commit && pre-commit install).Apologies for not using the template on the original description; I have restructured it above and added the changelog and AUTHORS entries that were genuinely missing. Disclosure: written with AI assistance (Claude Code); I ran the reproduction and the tests myself.